Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Button Refactors Final Final #16772

Merged
merged 6 commits into from
Aug 2, 2023
Merged

Button Refactors Final Final #16772

merged 6 commits into from
Aug 2, 2023

Conversation

J-Son89
Copy link
Contributor

@J-Son89 J-Son89 commented Jul 24, 2023

fixes: #16535

This pr updates the use of the button component.
I renamed a lot of props to align better with Figma and best practices
i.e
prop names
icon -> icon-only?
before -> icon-left
after -> icon-right
above -> icon-top
style -> container-style
disabled -> disabled?

removed all uses of combined types, e.g blur-bg -> type :grey & :background :blur

also I removed some internal styles as they were duplicates an not needed.

Updated all uses across the codebase to now use this api

Also fixed an alignment issue with the outline button variant where it was not sitting correctly

To Test: Each and every button

Additionally I found a number of uses of customization color for in the application that were not set correctly, this meant on first login the user's color was not showing. Now it is using the right sub for data and should show 👍

@J-Son89 J-Son89 self-assigned this Jul 24, 2023
@@ -3,101 +3,87 @@

(defn custom-color-type
[customization-color icon-only?]
{:icon-color colors/white
:icon-secondary-color colors/white-opa-70
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icon-secondary-color is not needed as the icon is the label color for icon only variant 👍

@@ -8,39 +8,35 @@
:bottom 0})

(defn icon-style
[{:keys [icon-container-size icon-background-color icon-container-rounded?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icon background color is not actually a thing afaik so I removed 👍

@status-im-auto
Copy link
Member

status-im-auto commented Jul 24, 2023

Jenkins Builds

Click to see older builds (18)
Commit #️⃣ Finished (UTC) Duration Platform Result
78aed34 #1 2023-07-24 16:16:42 ~3 min tests 📄log
✔️ 78aed34 #1 2023-07-24 16:19:17 ~5 min android-e2e 🤖apk 📲
✔️ 2c11d37 #2 2023-07-24 16:24:51 ~5 min android-e2e 🤖apk 📲
✔️ 2c11d37 #2 2023-07-24 16:26:17 ~6 min ios 📱ipa 📲
✔️ 2c11d37 #2 2023-07-24 16:28:09 ~8 min android 🤖apk 📲
✔️ 2c11d37 #2 2023-07-24 16:28:43 ~9 min tests 📄log
✔️ b81ad3e #3 2023-07-25 11:06:16 ~5 min android-e2e 🤖apk 📲
✔️ b81ad3e #3 2023-07-25 11:07:25 ~6 min ios 📱ipa 📲
✔️ b81ad3e #3 2023-07-25 11:07:51 ~6 min android 🤖apk 📲
✔️ b81ad3e #3 2023-07-25 11:10:39 ~9 min tests 📄log
✔️ 85e3f2a #4 2023-07-28 23:32:44 ~5 min android 🤖apk 📲
✔️ 85e3f2a #4 2023-07-28 23:33:38 ~6 min ios 📱ipa 📲
✔️ 85e3f2a #4 2023-07-28 23:35:10 ~8 min android-e2e 🤖apk 📲
✔️ 85e3f2a #4 2023-07-28 23:35:52 ~8 min tests 📄log
✔️ 945afe9 #5 2023-07-31 08:34:05 ~5 min android-e2e 🤖apk 📲
✔️ 945afe9 #5 2023-07-31 08:34:10 ~5 min android 🤖apk 📲
✔️ 945afe9 #5 2023-07-31 08:37:17 ~8 min tests 📄log
✔️ 945afe9 #5 2023-07-31 08:37:35 ~9 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 396b934 #6 2023-08-01 13:34:51 ~6 min android-e2e 🤖apk 📲
✔️ 396b934 #6 2023-08-01 13:37:26 ~8 min ios 📱ipa 📲
✔️ 396b934 #6 2023-08-01 13:38:55 ~10 min tests 📄log
✔️ 396b934 #6 2023-08-01 13:39:16 ~10 min android 🤖apk 📲
✔️ d8d5c41 #7 2023-08-02 14:52:40 ~8 min android-e2e 🤖apk 📲
✔️ d8d5c41 #7 2023-08-02 14:53:40 ~9 min android 🤖apk 📲
✔️ d8d5c41 #7 2023-08-02 14:56:00 ~11 min ios 📱ipa 📲
✔️ d8d5c41 #7 2023-08-02 15:01:26 ~17 min tests 📄log

@J-Son89 J-Son89 force-pushed the jc/refactor-button-follow-up branch from 78aed34 to 2c11d37 Compare July 24, 2023 16:19
:padding-horizontal (when-not (or icon-only? icon-left icon-right)
(case size
56 16
56 (if border-color 10 11)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes padding for icon above variant

40 9
32 5
3))
40 (if border-color 8 9)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes alignment for outline variant

:type type
:size 32
:accessibility-label accessibility-label
:background icon-background}
Copy link
Contributor Author

@J-Son89 J-Son89 Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a better solution for this, however this component needs to be revisited as it can be much simpler if we encapsulate the colors/designs correctly in the component 👍

@@ -1,7 +1,5 @@
(ns quo2.components.record-audio.record-audio.buttons.record-button
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if anyone feels like doing some refactoring, these components are in the wrong part of the codebase :) should be aligned with figma file structure convention 👍

@@ -63,14 +63,14 @@
[extra-action-view extra-action extra-text extra-action-selected?]
[rn/view {:style style/buttons-container}
[quo/button
{:type :grey
:style {:flex 0.48} ;;WUT? 0.48 , whats that ?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this WUT component is also some other work that should be address. This is acutally another component in the design system that should be added and used instead ->
#15006 still relevant?

Copy link
Contributor

@ilmotta ilmotta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @J-Son89. The code looks good.

I experimented with the button component in the preview area and found some rendering issues, but I haven't checked if they existed before. I'm approving the PR because I think you'll either fix them in this PR or create issues to be fixed in the near future :)

  1. Choosing a customization color doesn't change the button color. Maybe a problem just with the preview code, not the component itself.
  2. When the show-icon-top option is true, the button looks like Fig 1. It gets worse the smaller the button's size.
  3. When the show-icon-top and show-icon-right options are true, the button looks like Fig 2. If this combination of states are not possible, I think the code should reflect this. So ideally, only the possible variations in Figma should be rendered, and non-supported variations should just default to a good default state. Better have something correctly rendered per design, than something clearly broken. This I think should be part of our recommendations for writing components.
Figure 1
Figure 2

:padding-left (when-not (or icon-only? icon-left)
(case size
56 16
56 nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit weird to see a condition that returns nil, but I understand you probably kept 56 here to be perfectly symmetrical with the other cases that do have a non-nil value for 56.

Still, it's a bit rare to see in Clojure a case condition that explicitly returns nil because you can just add a default condition. Having the default condition is also more robust in the long run because the default value will prevent the code from throwing an exception when no size matches.

I know I keep repeating this, but case calls should have a default value of nil most of the time (I mean in particular for status-mobile, a CLJS-RN app). Imagine a designer adds a new size to Figma and that we devs update the majority of the code to handle this new size, but we fail to update certain case calls that don't have default values (such as the cases in this namespace) and boom, the code throws an exception.

Even if we gracefully handle the exception (which we don't now), I believe most users prefer to not see failures in apps, and are more receptive to a styling issue if it's not grotesque of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, well padding-left will overwrite it otherwise - sure I can update the default case to nil 👍 but there is default values here? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point @J-Son89. If there's no default value, the ideal scenario to me is to log and return nil. Unmatched conditions that will look wrong shouldn't go unnoticed. Of course, one may argue that nobody will see the logs, but it surely helps devs to more proactively catch these issues instead of real users, which would be bad.

Trusting that the code will pass only valid sizes now and in the future is a recipe for user bugs with case, so I would avoid that.

This idea to return nil to avoid the exception with case and log something if the unmatched case is unexpected is a good practice in general, so I think it doesn't hurt to do it. To be fair, I haven't done this in status-mobile, but I'll keep this practice in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh well this is quite specific to setting up this component because here padding horizontal can clash with padding left and padding right. Ideally this code could be written to just avoid the use of padding horizontal here but that would involve me redesigning the button styling and so I don't want to invest that effort as there is a lot variations to check and I have invested a lot of time in this component already. But there is definitely a cleaner approach here

:disabled (not= state :valid)})
(def button-view-profile
{:margin-top 24
:width "100%"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suspicious 100%?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine, I checked with Figma and it looks correct. Flex stretches the wrong way and I would have to adjust this component more than is necessary for this pr otherwise so it's the safest approach for what was already a misuse - it had a fixed width which was wrong 👍

@J-Son89
Copy link
Contributor Author

J-Son89 commented Jul 25, 2023

Great work @J-Son89. The code looks good.

I experimented with the button component in the preview area and found some rendering issues, but I haven't checked if they existed before. I'm approving the PR because I think you'll either fix them in this PR or create issues to be fixed in the near future :)

  1. Choosing a customization color doesn't change the button color. Maybe a problem just with the preview code, not the component itself.
  2. When the show-icon-top option is true, the button looks like Fig 1. It gets worse the smaller the button's size.
  3. When the show-icon-top and show-icon-right options are true, the button looks like Fig 2. If this combination of states are not possible, I think the code should reflect this. So ideally, only the possible variations in Figma should be rendered, and non-supported variations should just default to a good default state. Better have something correctly rendered per design, than something clearly broken. This I think should be part of our recommendations for writing components.

Figure 1
Figure 2

@ilmotta - not entirely sure what we should do here. These are both misconfigurations of the component. When icon-above is set there is no need for icon-right or icon-left (also the size can only be 56) - I can update the component a bit to handle this 👍

Alternatively we can handle this in the preview screens? which is better. I am not sure we should build in this complexity to our quo2 components, I think as long as we're matching a configuration from figma then it should be acceptable 🤔

@J-Son89
Copy link
Contributor Author

J-Son89 commented Jul 25, 2023

Great work @J-Son89. The code looks good.
I experimented with the button component in the preview area and found some rendering issues, but I haven't checked if they existed before. I'm approving the PR because I think you'll either fix them in this PR or create issues to be fixed in the near future :)

  1. Choosing a customization color doesn't change the button color. Maybe a problem just with the preview code, not the component itself.
  2. When the show-icon-top option is true, the button looks like Fig 1. It gets worse the smaller the button's size.
  3. When the show-icon-top and show-icon-right options are true, the button looks like Fig 2. If this combination of states are not possible, I think the code should reflect this. So ideally, only the possible variations in Figma should be rendered, and non-supported variations should just default to a good default state. Better have something correctly rendered per design, than something clearly broken. This I think should be part of our recommendations for writing components.

Figure 1
Figure 2

@ilmotta - not entirely sure what we should do here. These are both misconfigurations of the component. When icon-above is set there is no need for icon-right or icon-left (also the size can only be 56) - I can update the component a bit to handle this 👍

Alternatively we can handle this in the preview screens? which is better. I am not sure we should build in this complexity to our quo2 components, I think as long as we're matching a configuration from figma then it should be acceptable 🤔

Perhaps the preview screens could provide some validation for misconfigurations or even better if we could have some sort of types validation to our components, this would be a better approach imo. It seems like adding too much guard rails into the code of our design systems components adds unnecessary complexity for little benefit of what should be well designed code and aware developers that understand how we can best leverage using a design system approach.

@J-Son89 J-Son89 force-pushed the jc/refactor-button-follow-up branch from 2c11d37 to b81ad3e Compare July 25, 2023 11:00
@ilmotta
Copy link
Contributor

ilmotta commented Jul 25, 2023

not entirely sure what we should do here.

You mean the 3rd point? I think 1 is a bug in the preview screen and 2 is a bug in the component implementation, but I haven't checked the code to be sure.

@J-Son89, you are correct and I agree the complexity of the component code can grow, although marginally in my (single) experience with the list-items.community.view. I think this subject is interesting and full of nuances that it can be a GH discussion outside this PR. I'll move it there so as to not block this PR in any way :)

@J-Son89
Copy link
Contributor Author

J-Son89 commented Jul 25, 2023

Yeah I think this is best to have as a discussion first :)

@ulisesmac
Copy link
Contributor

Hey @J-Son89
when you are in a fresh install and you click the sign in option, then if you scan a valid but expired QR code, the "Try again" button has no color. I think that's related to the previous refactor. Could you please confirm?

thanks!

@J-Son89
Copy link
Contributor Author

J-Son89 commented Jul 27, 2023

Hey @J-Son89 when you are in a fresh install and you click the sign in option, then if you scan a valid but expired QR code, the "Try again" button has no color. I think that's related to the previous refactor. Could you please confirm?

thanks!

will check, thanks @ulisesmac !

@J-Son89 J-Son89 force-pushed the jc/refactor-button-follow-up branch 2 times, most recently from 85e3f2a to 945afe9 Compare July 31, 2023 08:28
@J-Son89 J-Son89 assigned churik and unassigned churik Jul 31, 2023
@Francesca-G
Copy link

ISSUE 5 Wrong color of composer buttons

Not sure about this one, need confirming from @Francesca-G

Outline color of the button itself looks right but I can confirm the color of the icons seems darker than it should be, the correct color is Neutral/Solid/50

Actual result:

photo_2023-08-01 14 19 15

Expected result:

Figma

Messages for Mobile – Figma 2023-08-01 14-18-48

@J-Son89
Copy link
Contributor Author

J-Son89 commented Aug 1, 2023

ISSUE 1 Enable notifications and Start using status buttons are missing background when user has signed in by syncing

  • Fixed and should be right now

ISSUE 2 Wrong buttons' colours in Activity Center

  • Fixed and should be right now

ISSUE 3 Unread counter brakes layout of AC icon

  • Fixed and should be right now

ISSUE 4 Broken (stretched) cancel reply button

  • Fixed and should be right now

ISSUE 5 Wrong color of composer buttons
This is actually an issue with the design system. I have since reached out to @mariocnovoa as they need to update the button component in the design system and then I will use the correct configuration once it is ready.
The previous implementation in code was an override "hack" to get it right and we want to use the correct approach which is via configuration - basically what this pr is doing for all of uses of the button.

As such I think it is best to leave for now and I can do in a follow up. If you would like more details on this let's discuss off of Github as it will be easier to explain in details :)

ISSUE 6 Wrong buttons' background on community home screen (Android)

  • As mentioned above, it should look different with the banner image which is mandatory in desktop community creation

@pavloburykh
Copy link
Contributor

pavloburykh commented Aug 1, 2023

@J-Son89 thanx for the fixes! Will check and provide feedback.

Regarding

ISSUE 6 Wrong buttons' background on community home screen (Android)

As mentioned above, it should look different with the banner image which is mandatory in desktop community creation

I have checked community that has background image set and button's background still does not look correct to me.

photo_2023-08-01 17 23 07

For comparison here is how it looks on IOS

photo_2023-08-01 17 29 37

Here how it looks in Figma

Communities for Mobile – Figma 2023-08-01 17-30-36

Seems like both IOS and Android buttons' background colour differ from design in this particular case. I guess we need another input from @Francesca-G on this issue.

@J-Son89
Copy link
Contributor Author

J-Son89 commented Aug 1, 2023

Yeah, there is some blur on this so it really depends on the background to match with Figma. If @Francesca-G can check that would be great :)

@status-im-auto
Copy link
Member

80% of end-end tests have passed

Total executed tests: 40
Failed tests: 8
Passed tests: 32
IDs of failed tests: 702732,703133,703495,703297,703202,702807,702731,702808 

Failed tests (8)

Click to expand
  • Rerun failed tests

  • Class TestCommunityOneDeviceMerged:

    1. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] Restoring communities issue: 16787; restoring contacts issue: 15500]]

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_pin_messages, id: 702731

    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] Pin feature is in development]]

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_pin_messages, id: 702732

    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] Pin feature is in development]]

    2. test_group_chat_mute_chat, id: 703495

    Test setup failed: critical/chats/test_group_chat.py:186: in prepare_devices
        self.loop.run_until_complete(
    /usr/lib/python3.10/asyncio/base_events.py:646: in run_until_complete
        return future.result()
    __init__.py:44: in run_in_parallel
        returns.append(await k)
    /usr/lib/python3.10/concurrent/futures/thread.py:58: in run
        result = self.fn(*self.args, **self.kwargs)
    ../views/home_view.py:377: in handle_contact_request
        self.close_activity_centre.click()
    ../views/base_element.py:91: in click
        self.find_element().click()
    ../views/base_element.py:80: in find_element
        raise NoSuchElementException(
     Device 3: Button by accessibility id: `close-activity-center` is not found on the screen
    



    3. test_group_chat_send_image_save_and_share, id: 703297

    Test setup failed: critical/chats/test_group_chat.py:186: in prepare_devices
        self.loop.run_until_complete(
    /usr/lib/python3.10/asyncio/base_events.py:646: in run_until_complete
        return future.result()
    __init__.py:44: in run_in_parallel
        returns.append(await k)
    /usr/lib/python3.10/concurrent/futures/thread.py:58: in run
        result = self.fn(*self.args, **self.kwargs)
    ../views/home_view.py:377: in handle_contact_request
        self.close_activity_centre.click()
    ../views/base_element.py:91: in click
        self.find_element().click()
    ../views/base_element.py:80: in find_element
        raise NoSuchElementException(
     Device 3: Button by accessibility id: `close-activity-center` is not found on the screen
    



    4. test_group_chat_reactions, id: 703202

    Test setup failed: critical/chats/test_group_chat.py:186: in prepare_devices
        self.loop.run_until_complete(
    /usr/lib/python3.10/asyncio/base_events.py:646: in run_until_complete
        return future.result()
    __init__.py:44: in run_in_parallel
        returns.append(await k)
    /usr/lib/python3.10/concurrent/futures/thread.py:58: in run
        result = self.fn(*self.args, **self.kwargs)
    ../views/home_view.py:377: in handle_contact_request
        self.close_activity_centre.click()
    ../views/base_element.py:91: in click
        self.find_element().click()
    ../views/base_element.py:80: in find_element
        raise NoSuchElementException(
     Device 3: Button by accessibility id: `close-activity-center` is not found on the screen
    



    5. test_group_chat_join_send_text_messages_push, id: 702807

    Device 2: Find Button by accessibility id: close-activity-center
    Device 2: Tap on found: Button

    Test setup failed: critical/chats/test_group_chat.py:186: in prepare_devices
        self.loop.run_until_complete(
    /usr/lib/python3.10/asyncio/base_events.py:646: in run_until_complete
        return future.result()
    __init__.py:44: in run_in_parallel
        returns.append(await k)
    /usr/lib/python3.10/concurrent/futures/thread.py:58: in run
        result = self.fn(*self.args, **self.kwargs)
    ../views/home_view.py:377: in handle_contact_request
        self.close_activity_centre.click()
    ../views/base_element.py:91: in click
        self.find_element().click()
    ../views/base_element.py:80: in find_element
        raise NoSuchElementException(
     Device 3: Button by accessibility id: `close-activity-center` is not found on the screen
    



    Device sessions

    6. test_group_chat_offline_pn, id: 702808

    Test setup failed: critical/chats/test_group_chat.py:186: in prepare_devices
        self.loop.run_until_complete(
    /usr/lib/python3.10/asyncio/base_events.py:646: in run_until_complete
        return future.result()
    __init__.py:44: in run_in_parallel
        returns.append(await k)
    /usr/lib/python3.10/concurrent/futures/thread.py:58: in run
        result = self.fn(*self.args, **self.kwargs)
    ../views/home_view.py:377: in handle_contact_request
        self.close_activity_centre.click()
    ../views/base_element.py:91: in click
        self.find_element().click()
    ../views/base_element.py:80: in find_element
        raise NoSuchElementException(
     Device 3: Button by accessibility id: `close-activity-center` is not found on the screen
    



    Passed tests (32)

    Click to expand

    Class TestCommunityOneDeviceMerged:

    1. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    2. test_community_undo_delete_message, id: 702869
    Device sessions

    3. test_community_navigate_to_channel_when_relaunch, id: 702846
    Device sessions

    4. test_community_mute_community_and_channel, id: 703382
    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_several_images_send_reply, id: 703194
    Device sessions

    2. test_community_one_image_send_reply, id: 702859
    Device sessions

    3. test_community_emoji_send_copy_paste_reply, id: 702840
    Device sessions

    4. test_community_mark_all_messages_as_read, id: 703086
    Device sessions

    5. test_community_contact_block_unblock_offline, id: 702894
    Device sessions

    6. test_community_mentions_push_notification, id: 702786
    Device sessions

    7. test_community_message_delete, id: 702839
    Device sessions

    8. test_community_message_send_check_timestamps_sender_username, id: 702838
    Device sessions

    9. test_community_links_with_previews_github_youtube_twitter_gif_send_enable, id: 702844
    Device sessions

    10. test_community_message_edit, id: 702843
    Device sessions

    11. test_community_leave, id: 702845
    Device sessions

    12. test_community_unread_messages_badge, id: 702841
    Device sessions

    Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_activity_center_contact_request_accept_swipe_mark_all_as_read, id: 702851
    Device sessions

    2. test_activity_center_contact_request_decline, id: 702850
    Device sessions

    Class TestActivityMultipleDevicePR:

    1. test_activity_center_mentions, id: 702957
    Device sessions

    2. test_navigation_jump_to, id: 702936
    Device sessions

    3. test_activity_center_reply_read_unread_delete_filter_swipe, id: 702947
    Device sessions

    4. test_activity_center_admin_notification_accept_swipe, id: 702958
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_emoji_send_reply_and_open_link, id: 702782
    Device sessions

    2. test_1_1_chat_text_message_delete_push_disappear, id: 702733
    Device sessions

    3. test_1_1_chat_delete_via_long_press_relogin, id: 702784
    Device sessions

    4. test_1_1_chat_push_emoji, id: 702813
    Device sessions

    5. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    6. test_1_1_chat_is_shown_message_sent_delivered_from_offline, id: 702783
    Device sessions

    7. test_1_1_chat_mute_chat, id: 703496
    Device sessions

    8. test_1_1_chat_edit_message, id: 702855
    Device sessions

    9. test_1_1_chat_send_image_save_and_share, id: 703391
    Device sessions

    10. test_1_1_chat_message_reaction, id: 702730
    Device sessions

    @Francesca-G
    Copy link

    Yeah, there is some blur on this so it really depends on the background to match with Figma. If @Francesca-G can check that would be great :)

    @pavloburykh @J-Son89 I can confirm both buttons in the top page nav have a blur, so the color will slightly change based on the cover image.

    Screenshot 2023-08-01 alle 17 46 56 Screenshot 2023-08-01 alle 17 53 58

    Light mode: button color is White/40% with background blur effect Blur 40
    Dark mode: button color is Neutral/80 Transparent/40% with background blur effect Blur 40

    When a context menu or a drawer is open there's an overlay layer applied to the background page but that shouldn't affect the blur or the color of the buttons.

    Screenshot 2023-08-01 alle 17 47 07 Screenshot 2023-08-01 alle 17 54 05

    @pavloburykh
    Copy link
    Contributor

    When a context menu or a drawer is open there's an overlay layer applied to the background page but that shouldn't affect the blur or the color of the buttons.

    Thank you for your review @Francesca-G

    @J-Son89 based on that, the behaviour on Android device looks incorrect, because overlay layer affects the blur and color of the buttons.

    telegram-cloud-document-2-5352946714577743856.mp4

    @status-im-auto
    Copy link
    Member

    80% of end-end tests have passed

    Total executed tests: 5
    Failed tests: 1
    Passed tests: 4
    
    IDs of failed tests: 703495 
    

    Failed tests (1)

    Click to expand
  • Rerun failed tests

  • Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_mute_chat, id: 703495

    Device 2: Click until `ChatMessageInput` by `accessibility id`: `chat-message-input` will be presented
    Device 2: Looking for a message by text: Chat is unmuted now

    critical/chats/test_group_chat.py:607: in test_group_chat_mute_chat
        self.errors.verify_no_errors()
    base_test_case.py:183: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     New messages counter near chat name is not shown after unmute
    



    Device sessions

    Passed tests (4)

    Click to expand

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_reactions, id: 703202
    Device sessions

    2. test_group_chat_join_send_text_messages_push, id: 702807
    Device sessions

    3. test_group_chat_offline_pn, id: 702808
    Device sessions

    4. test_group_chat_send_image_save_and_share, id: 703297
    Device sessions

    @pavloburykh
    Copy link
    Contributor

    @J-Son89 ISSUEs 1, 2, 3, 4 are fixed thank you. As agreed ISSUE 5 will be logged as a followup. So waiting for your feedback regarding ISSUE 6

    @J-Son89
    Copy link
    Contributor Author

    J-Son89 commented Aug 2, 2023

    Awesome, thanks @pavloburykh. I'm looking into it today and will keep you updated!

    @J-Son89
    Copy link
    Contributor Author

    J-Son89 commented Aug 2, 2023

    @pavloburykh, @Francesca-G regarding to issue 6. Would it be okay to leave this as a follow up work? It is specific to the communities page and is only there as the buttons are correctly set now. There is some work that needs to be done on that scrolling page and will be cleaner if it is done separately.
    what do you think?
    I can create any corresponding issues related to that 👍

    Or it could be done as part of the work related to 1.24 design review - https://www.figma.com/file/Tf5nfkYvpbnNCo4rKLK7lS/Feedback-for-Mobile?type=design&node-id=3674-338290&mode=design&t=1ZmsbGWnOi95yo11-4
    as it will touch that part.
    This pr technically fixes some parts of that design review but needs some other adjustments too

    Copy link

    @Francesca-G Francesca-G left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @J-Son89 follow up required :) thank you!

    @pavloburykh
    Copy link
    Contributor

    @J-Son89 thank you! I will create separate tickets for ISSUE 5 and 6. PR is ready for merge.

    @J-Son89
    Copy link
    Contributor Author

    J-Son89 commented Aug 2, 2023

    @J-Son89 thank you! I will create separate tickets for ISSUE 5 and 6. PR is ready for merge.

    Thanks @pavloburykh, you can assign issue 5 to me and I can start it this week 👍

    @J-Son89 J-Son89 force-pushed the jc/refactor-button-follow-up branch from 396b934 to d8d5c41 Compare August 2, 2023 14:44
    @J-Son89 J-Son89 merged commit d26db93 into develop Aug 2, 2023
    @J-Son89 J-Son89 deleted the jc/refactor-button-follow-up branch August 2, 2023 15:03
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    Archived in project
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    Button Component
    7 participants